Skip to content

Hide analyzed panel when it's empty#1835

Merged
koesie10 merged 1 commit intomainfrom
koesie10/hide-analyzed-when-failed
Dec 5, 2022
Merged

Hide analyzed panel when it's empty#1835
koesie10 merged 1 commit intomainfrom
koesie10/hide-analyzed-when-failed

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Dec 5, 2022

This will hide the "Analyzed" panel when there are no scanned repos and it's completely empty.

When all three panels are empty, this will also hide the search bar and filters, and will skip rendering anything for the panels.

Before:

Screenshot 2022-12-05 at 12 07 27

After:

Screenshot 2022-12-05 at 12 07 46

Screenshot 2022-12-05 at 14 03 33

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 added the secexp label Dec 5, 2022
@robertbrignull
Copy link
Copy Markdown
Contributor

For the third screenshot where there are no panels to display, could we add the "no repositories to analyse" text? I worry it's not clear why nothing is being displayed, and I worry people would think the page is broken.

I'm not sure it's possible to hit this case since the API won't let you create a variant analysis with no scanned or skipped repos, but I appreciate the UI still needs to do something in this case.

"ts-jest",
{
tsconfig: "src/view/tsconfig.spec.json",
tsconfig: "<rootDir>/tsconfig.spec.json",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering why does this change need to happen for this PR? Could this be moved to a separate PR so it's not mixed in with unrelated changes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've created #1836 for this.

@robertbrignull
Copy link
Copy Markdown
Contributor

The general idea of this PR looks great to me. Showing only the relevant tabs will make the view much clearer.

This will hide the "Analyzed" panel when there are no scanned repos and
it's completely empty.

When all three panels are empty, this will also hide the search bar and
filters, and will skip rendering anything for the panels.
@koesie10 koesie10 force-pushed the koesie10/hide-analyzed-when-failed branch from c456fb0 to 96850e4 Compare December 5, 2022 13:06
@koesie10
Copy link
Copy Markdown
Member Author

koesie10 commented Dec 5, 2022

Thanks for the review!

For the third screenshot where there are no panels to display, could we add the "no repositories to analyse" text? I worry it's not clear why nothing is being displayed, and I worry people would think the page is broken.

I'm not sure it's possible to hit this case since the API won't let you create a variant analysis with no scanned or skipped repos, but I appreciate the UI still needs to do something in this case.

I have updated the PR so we now show the warnings that are normally also shown. I have updated the screenshot in the PR description. I don't believe we can get into this state normally, but could it be that we get in this state when the processing job fails at some point?

@koesie10 koesie10 marked this pull request as ready for review December 5, 2022 13:10
@koesie10 koesie10 requested a review from a team as a code owner December 5, 2022 13:10
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@koesie10 koesie10 merged commit b91e31c into main Dec 5, 2022
@koesie10 koesie10 deleted the koesie10/hide-analyzed-when-failed branch December 5, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants